-
Notifications
You must be signed in to change notification settings - Fork 428
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MUC Light ODBC #1093
MUC Light ODBC #1093
Conversation
-include("jlib.hrl"). | ||
-include("mod_muc_light.hrl"). | ||
|
||
-define(esc(T), ejabberd_odbc:escape(T)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
Invalid macro name esc on line 62. Use UPPER_CASE.
@@ -304,7 +311,7 @@ add_rooms_to_roster(Acc, UserUS) -> | |||
children = [#xmlcdata{ content = RoomVersion }] }] | |||
}, | |||
[Item | Acc0] | |||
end, Acc, get_rooms_info(lists:sort(?BACKEND:get_user_rooms(UserUS)))). | |||
end, Acc, get_rooms_info(lists:sort(?BACKEND:get_user_rooms(UserUS, undefined)))). | |||
|
|||
-spec process_iq_get(Acc :: any(), From :: #jid{}, To :: #jid{}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
The spec in line 316 uses a record, please define a type for the record and use that instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Elvis here.
{selected, _, [{<<"0">>}]} -> | ||
throw({aborted, Reason}); | ||
{selected, _, [{<<"1">>}]} -> | ||
case NodeCandidate of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
The expression on line 316 and column 21 is nested beyond the maximum level of 3.
aff_insert_sql(RoomID, UserU, UserS, Aff)); | ||
false -> | ||
{updated, _} = ejabberd_odbc:sql_query_t( | ||
aff_update_sql(RoomID, UserU, UserS, Aff)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
Line 516 is too long: aff_update_sql(RoomID, UserU, UserS, Aff)).
true -> odbc; | ||
false -> mnesia | ||
end, | ||
ct:pal("Testing MUC Light with backend: ~p", [Backend]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
Remove the debug call to ct:pal/2 on line 133.
@@ -457,7 +464,7 @@ handle_disco_info_get(From, To, DiscoInfo) -> | |||
-spec handle_disco_items_get(From :: jid(), To :: jid(), DiscoItems :: #disco_items{}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
The spec in line 464 uses a record, please define a type for the record and use that instead.
[{{UserU, UserS}, aff_db2atom(Aff)} || {UserU, UserS, Aff} <- AffUsersDB]), | ||
case mod_muc_light_utils:change_aff_users(AffUsers, AffUsersChanges) of | ||
{ok, NewAffUsers, AffUsersChanged, JoiningUsers, _LeavingUsers} -> | ||
case CheckFun(RoomUS, NewAffUsers) of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
The expression on line 503 and column 21 is nested beyond the maximum level of 3.
@@ -336,7 +343,7 @@ process_iq_set(_Acc, #jid{ lserver = FromS } = From, To, #iq{} = IQ) -> | |||
true -> | |||
{stop, {error, ?ERR_BAD_REQUEST}}; | |||
false -> | |||
ok = ?BACKEND:set_blocking(jid:to_lus(From), Items), | |||
ok = ?BACKEND:set_blocking(jid:to_lus(From), MUCHost, Items), | |||
?CODEC:encode(Blocking, From, jid:to_lus(To), | |||
fun(_, _, Packet) -> put(encode_res, Packet) end), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
The expression on line 348 and column 35 is nested beyond the maximum level of 3.
{selected, _, [{RoomID}]} = ejabberd_odbc:sql_query_t(get_room_id_sql(RoomU, RoomS)), | ||
lists:foreach( | ||
fun({{UserU, UserS}, Aff}) -> | ||
{updated, _} = ejabberd_odbc:sql_query_t(aff_insert_sql(RoomID, UserU, UserS, Aff)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
Line 325 is too long: {updated, _} = ejabberd_odbc:sql_query_t(aff_insert_sql(RoomID, UserU, UserS, Aff)).
@@ -19,6 +21,13 @@ | |||
|
|||
-define(RPC(M,F,A), escalus_ejabberd:rpc(M, F, A)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
Missing space right "," on line 22
create_room(From, FromUS, To, Create, OrigPacket); | ||
false -> | ||
true -> | ||
?CODEC:encode_error( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
Don't use macros (like CODEC on line 219) as module names.
RoomsPerUser -> | ||
length(?BACKEND:get_user_rooms(FromUS, To#jid.lserver)) < RoomsPerUser | ||
end, | ||
if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
Replace the 'if' expression on line 215 with a 'case' expression or function clauses.
{selected, _, AffUsersDB} = ejabberd_odbc:sql_query( | ||
MainHost, mod_muc_light_db_odbc_sql:select_affs(RoomID)), | ||
AffUsers = [{{UserU, UserS}, aff_db2atom(Aff)} || {UserU, UserS, Aff} <- AffUsersDB], | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
Line 270 has 12 trailing whitespace characters.
create_room(From, FromUS, To, Create, OrigPacket); | ||
false -> | ||
true -> | ||
?CODEC:encode_error( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
Remove the dynamic function call on line 219. Only modules that define callbacks should make dynamic calls.
@@ -19,6 +21,13 @@ | |||
|
|||
-define(RPC(M,F,A), escalus_ejabberd:rpc(M, F, A)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
Missing space right "," on line 22
-export([select_blocking/2, select_blocking_cnt/3, insert_blocking/4, | ||
delete_blocking/4, delete_blocking/2]). | ||
|
||
-define(esc(T), ejabberd_odbc:escape(T)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
Invalid macro name esc on line 37. Use UPPER_CASE.
@@ -19,6 +21,13 @@ | |||
|
|||
-define(RPC(M,F,A), escalus_ejabberd:rpc(M, F, A)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
Missing space right "," on line 22
RoomsPerUser -> | ||
length(?BACKEND:get_user_rooms(FromUS, To#jid.lserver)) < RoomsPerUser | ||
end, | ||
if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
Replace the 'if' expression on line 215 with a 'case' expression or function clauses.
@@ -152,7 +152,7 @@ filter_out_prevented(FromUS, {RoomU, MUCServer} = RoomUS, AffUsers) -> | |||
end, | |||
if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
Replace the 'if' expression on line 153 with a 'case' expression or function clauses.
{selected, _, []} -> | ||
throw({aborted, Reason}); | ||
{selected, _, [_]} -> | ||
case NodeCandidate of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
The expression on line 337 and column 21 is nested beyond the maximum level of 3.
create_room(From, FromUS, To, Create, OrigPacket); | ||
false -> | ||
true -> | ||
?CODEC:encode_error( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
Don't use macros (like CODEC on line 219) as module names.
lists:map( | ||
fun(RoomUS) -> | ||
{RoomUS, modify_aff_users_transaction( | ||
RoomUS, [{UserUS, none}], fun(_,_) -> ok end, Version)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
Missing space right "," on line 383
NotBlocked andalso RoomsBelowLimit -> | ||
[AffUser | filter_out_loop(FromUS, MUCServer, BlockingQuery, RoomsPerUser, RAffUsers)]; | ||
true -> | ||
filter_out_loop(FromUS, MUCServer, BlockingQuery, RoomsPerUser, RAffUsers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
Line 183 has 5 trailing whitespace characters.
@@ -558,14 +565,15 @@ find_room_pos(_, [], _) -> {error, item_not_found}. | |||
BlockingReq :: {get | set, #blocking{}}) -> | |||
{error, bad_request} | ok. | |||
handle_blocking(From, To, {get, #blocking{} = Blocking}) -> | |||
?CODEC:encode({get, Blocking#blocking{ items = ?BACKEND:get_blocking(jid:to_lus(From)) }}, | |||
BlockingItems = ?BACKEND:get_blocking(jid:to_lus(From), To#jid.lserver), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
Remove the dynamic function call on line 568. Only modules that define callbacks should make dynamic calls.
get_user_rooms(UserJID) -> | ||
?BACKEND:get_user_rooms(jid:to_lus(UserJID)). | ||
get_user_rooms(UserJID, Domain) -> | ||
?BACKEND:get_user_rooms(jid:to_lus(UserJID), Domain). | ||
|
||
name_of_room_with_jid(RoomJID) -> | ||
case ?BACKEND:get_info(RoomJID) of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
Remove the dynamic function call on line 186. Only modules that define callbacks should make dynamic calls.
From, jid:to_lus(To), fun ejabberd_router:route/3); | ||
handle_blocking(From, To, {set, #blocking{ items = Items }} = BlockingReq) -> | ||
case lists:any(fun({_, _, {WhoU, WhoS}}) -> WhoU =:= <<>> orelse WhoS =:= <<>> end, Items) of | ||
true -> | ||
{error, bad_request}; | ||
false -> | ||
ok = ?BACKEND:set_blocking(jid:to_lus(From), Items), | ||
ok = ?BACKEND:set_blocking(jid:to_lus(From), To#jid.lserver, Items), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
Don't use macros (like BACKEND on line 576) as module names.
@@ -304,7 +311,7 @@ add_rooms_to_roster(Acc, UserUS) -> | |||
children = [#xmlcdata{ content = RoomVersion }] }] | |||
}, | |||
[Item | Acc0] | |||
end, Acc, get_rooms_info(lists:sort(?BACKEND:get_user_rooms(UserUS)))). | |||
end, Acc, get_rooms_info(lists:sort(?BACKEND:get_user_rooms(UserUS, undefined)))). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
Remove the dynamic function call on line 314. Only modules that define callbacks should make dynamic calls.
filter_out_loop( | ||
FromUS, MUCServer, BlockingQuery, RoomsPerUser, [{UserUS, _} = AffUser | RAffUsers]) -> | ||
NotBlocked = case (BlockingQuery == undefined orelse UserUS =:= FromUS) of | ||
false -> ?BACKEND:get_blocking(UserUS, MUCServer, BlockingQuery) == allow; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
Remove the dynamic function call on line 172. Only modules that define callbacks should make dynamic calls.
get_user_rooms(UserJID) -> | ||
?BACKEND:get_user_rooms(jid:to_lus(UserJID)). | ||
get_user_rooms(UserJID, Domain) -> | ||
?BACKEND:get_user_rooms(jid:to_lus(UserJID), Domain). | ||
|
||
name_of_room_with_jid(RoomJID) -> | ||
case ?BACKEND:get_info(RoomJID) of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
Don't use macros (like BACKEND on line 186) as module names.
infinity -> | ||
true; | ||
RoomsPerUser -> | ||
length(?BACKEND:get_user_rooms(FromUS, To#jid.lserver)) < RoomsPerUser |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How difficult would it be to implement ?BACKEND:get_user_rooms_count
? With count on the backend side?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not difficult at all and actually I was considering it. Let's do it!
FromUS = jid:to_lus(From), | ||
case RoomsPerUser == infinity orelse length(?BACKEND:get_user_rooms(FromUS)) < RoomsPerUser of | ||
MayCreate = case get_opt(To#jid.lserver, rooms_per_user, ?DEFAULT_ROOMS_PER_USER) of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move the whole case
to a dedicated function with meaningful name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, why not.
@@ -304,7 +311,7 @@ add_rooms_to_roster(Acc, UserUS) -> | |||
children = [#xmlcdata{ content = RoomVersion }] }] | |||
}, | |||
[Item | Acc0] | |||
end, Acc, get_rooms_info(lists:sort(?BACKEND:get_user_rooms(UserUS)))). | |||
end, Acc, get_rooms_info(lists:sort(?BACKEND:get_user_rooms(UserUS, undefined)))). | |||
|
|||
-spec process_iq_get(Acc :: any(), From :: #jid{}, To :: #jid{}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Elvis here.
(RoomsPerUser == infinity orelse length(?BACKEND:get_user_rooms(UserUS)) < RoomsPerUser) of | ||
true -> [AffUser | filter_out_loop(FromUS, BlockingQuery, RoomsPerUser, RAffUsers)]; | ||
false -> filter_out_loop(FromUS, BlockingQuery, RoomsPerUser, RAffUsers) | ||
filter_out_loop( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about following line split:
filter_out_loop(FromUS, MUCServer, BlockingQuery,
RoomsPerUser, [{UserUS, _} = AffUser | RAffUsers])
Look the comment "source code" as Github renders it incorrectly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
?CODEC:encode_error({error, item_not_found}, From, To, OrigPacket, | ||
fun ejabberd_router:route/3) | ||
mod_muc_light_codec_backend:encode_error( | ||
{error, item_not_found}, From, To, OrigPacket, fun ejabberd_router:route/3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
The expression on line 486 and column 70 is nested beyond the maximum level of 3.
ok = ?BACKEND:set_blocking(jid:to_lus(From), Items), | ||
?CODEC:encode(Blocking, From, jid:to_lus(To), | ||
ok = mod_muc_light_db_backend:set_blocking(jid:to_lus(From), MUCHost, Items), | ||
mod_muc_light_codec_backend:encode(Blocking, From, jid:to_lus(To), | ||
fun(_, _, Packet) -> put(encode_res, Packet) end), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
The expression on line 349 and column 35 is nested beyond the maximum level of 3.
@@ -223,39 +224,40 @@ process_packet(From, To, {ok, {_, #blocking{}} = Blocking}, OrigPacket) -> | |||
ok -> | |||
ok; | |||
Error -> | |||
?CODEC:encode_error(Error, From, To, OrigPacket, | |||
mod_muc_light_codec_backend:encode_error(Error, From, To, OrigPacket, | |||
fun ejabberd_router:route/3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
The expression on line 228 and column 54 is nested beyond the maximum level of 3.
Rooms -> | ||
RoomsInfo = get_rooms_info(lists:sort(Rooms)), | ||
RoomsPerPage = get_opt(To#jid.lserver, rooms_per_page, ?DEFAULT_ROOMS_PER_PAGE), | ||
case apply_rsm(RoomsInfo, length(RoomsInfo), | ||
page_service_limit(DiscoItems0#disco_items.rsm, RoomsPerPage)) of | ||
{ok, RoomsInfoSlice, RSMOut} -> | ||
DiscoItems = DiscoItems0#disco_items{ rooms = RoomsInfoSlice, rsm = RSMOut }, | ||
?CODEC:encode({get, DiscoItems}, From, jid:to_lus(To), | ||
mod_muc_light_codec_backend:encode({get, DiscoItems}, From, jid:to_lus(To), | ||
fun ejabberd_router:route/3); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
The expression on line 483 and column 35 is nested beyond the maximum level of 3.
process_iq_set(_Acc, #jid{ lserver = FromS } = From, To, #iq{} = IQ) -> | ||
MUCHost = gen_mod:get_module_opt_host(FromS, ?MODULE, ?DEFAULT_HOST), | ||
case {?CODEC:decode(From, To, IQ), get_opt(MUCHost, blocking, ?DEFAULT_BLOCKING)} of | ||
case {mod_muc_light_codec_backend:decode(From, To, IQ), | ||
get_opt(MUCHost, blocking, ?DEFAULT_BLOCKING)} of | ||
{{ok, {set, #blocking{ items = Items }} = Blocking}, true} -> | ||
case lists:any(fun({_, _, {WhoU, WhoS}}) -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
The expression on line 341 and column 28 is nested beyond the maximum level of 3.
|
||
mod_muc_light_codec_backend:encode_error({error, item_not_found}, | ||
From, To, OrigPacket, | ||
fun ejabberd_router:route/3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
The expression on line 486 and column 62 is nested beyond the maximum level of 3.
?CODEC:encode(Blocking, From, jid:to_lus(To), | ||
fun(_, _, Packet) -> put(encode_res, Packet) end), | ||
ok = mod_muc_light_db_backend:set_blocking(jid:to_lus(From), MUCHost, Items), | ||
RouteFun = fun(_, _, Packet) -> put(encode_res, Packet) end, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
The expression on line 347 and column 32 is nested beyond the maximum level of 3.
?CODEC:encode_error(Error, From, To, OrigPacket, | ||
fun ejabberd_router:route/3) | ||
mod_muc_light_codec_backend:encode_error(Error, From, To, OrigPacket, | ||
fun ejabberd_router:route/3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
The expression on line 228 and column 62 is nested beyond the maximum level of 3.
case lists:any(fun({_, _, {WhoU, WhoS}}) -> | ||
WhoU =:= <<>> orelse WhoS =:= <<>> | ||
end, Items) of | ||
case lists:any(fun({_, _, {WhoU, WhoS}}) -> WhoU =:= <<>> orelse WhoS =:= <<>> end, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
The expression on line 341 and column 28 is nested beyond the maximum level of 3.
Rooms -> | ||
RoomsInfo = get_rooms_info(lists:sort(Rooms)), | ||
RoomsPerPage = get_opt(To#jid.lserver, rooms_per_page, ?DEFAULT_ROOMS_PER_PAGE), | ||
case apply_rsm(RoomsInfo, length(RoomsInfo), | ||
page_service_limit(DiscoItems0#disco_items.rsm, RoomsPerPage)) of | ||
{ok, RoomsInfoSlice, RSMOut} -> | ||
DiscoItems = DiscoItems0#disco_items{ rooms = RoomsInfoSlice, rsm = RSMOut }, | ||
?CODEC:encode({get, DiscoItems}, From, jid:to_lus(To), | ||
mod_muc_light_codec_backend:encode({get, DiscoItems}, From, jid:to_lus(To), | ||
fun ejabberd_router:route/3); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Elvis:
The expression on line 482 and column 35 is nested beyond the maximum level of 3.
6ee2a47
to
382fa14
Compare
Good job! I'll merge it today. |
This PR adds basic ODBC backend for MUC Light.
It should be a subject of load testing, whether this backend needs optimisation, caching, stored procedures or any other improvements.